ESQL: Fix LOOKUP JOIN with limit #120411

alex-spies · 2025-01-17T17:23:26Z

This is like #115624, in spirit, but for LOOKUP JOIN.

For queries like

... | LOOKUP JOIN lookup_index ON key | LIMIT 10

the limit cannot be simply pushed past the join - but it can be duplicated past the join. The same currently happens with MV_EXPAND.

This PR solves this by leaving an explicit Limit plan node downstream from the Join (in addition to pushing down the limit), but marks it in a way that prevents being duplicated multiple times (which would cause infinite loops).

In contrast to the approach used for MV_EXPAND, the downstream limit is not internalized into the Join node, but instead we mark it with a boolean attribute that marks them as un-pushable past MvExpand and Joins. The fix for MV_EXPAND is updated and aligned with the approach for LOOKUP JOIN.

This approach was discussed with @luigidellaquila. I believe it is preferable rather than having a limit attribute on MvExpand and Join (+ subclasses), because it means that the logic of pushing down limits remains local to the Limit class, and the mapper remains oblivious to all this, too. Also, any optimization rules (current and future ones) that do anything with limits continue to work without additional special casing for MvExpand and Join. (PushDownAndCombineLimits is already affected by this in an edge case.)

alex-spies

I plan to add more optimizer tests, but the production code changes are complete, so I'm undrafting already.

elasticsearchmachine · 2025-01-21T16:41:12Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec

astefan · 2025-01-22T16:33:22Z

I've started playing with the code. Found something, putting it here, sorry for not digging through it due to time constraints. I've added some more rows to employees to make it more challenging (I hoped). Also, apologies for the messy test data:

{"index":{"_index":"employees","_id":1}}
{"text2":"red fox jumps over black dog", "emp_no":555, "languages": 1}
{"index":{"_index":"employees","_id":2}}
{"text2":"red fox", "emp_no":666, "languages": 2}
{"index":{"_index":"employees","_id":3}}
{"text2":"black dog", "emp_no":777, "languages": 3}
{"index":{"_index":"employees","_id":4}}
{"text2":null, "emp_no":0, "languages": null}
{"index":{"_index":"employees","_id":5}}
{"text2":"foo", "emp_no":[50005,50006], "languages": [2,3,120]}
{"index":{"_index":"employees","_id":6}}
{"text2":"bar", "emp_no":[50007,50008], "languages": [1,2,3]}
{"index":{"_index":"employees","_id":7}}
{"text2":null, "emp_no":[50009,50010], "languages": [3,5]}

This one results in

status: 500 org.elasticsearch.xpack.esql.EsqlIllegalArgumentException: unknown physical plan node [OrderExec]
        at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:244)
        at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.planProject(LocalExecutionPlanner.java:618)
        at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:209)
        at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.planLookupJoin(LocalExecutionPlanner.java:554)
        at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:235)
        at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.planTopN(LocalExecutionPlanner.java:351)
        at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:201)
        at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.planProject(LocalExecutionPlanner.java:618)
        at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:209)
        at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.planOutput(LocalExecutionPlanner.java:282)
        at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:239)
        at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:180)
        at org.elasticsearch.xpack.esql.plugin.ComputeService.runCompute(ComputeService.java:316)

alex-spies · 2025-01-22T19:04:49Z

Nice find @astefan . I can reproduce this, also with mv_expand.

However, the problem is already present on main (also for mv_expand). It is unrelated to this PR per se, and has to do with the fact that if there's more than one SORT, there's no LIMIT to be pushed into the upstream SORT, and thus we do not create a TopN out of it. For most commands, this is solved by pushing them down past the SORT, so that multiple SORTs end up next to each other and can be pruned.

I'll create a separate issue for this.

costin

LGTM

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Limit.java

luigidellaquila

Thanks Alex, LGTM!

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Limit.java

Use a helper function for all the similar as(...) calls for Limit.class.

bpintea

Nice. Left two highly optional notes.

...main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimits.java

alex-spies

I added a bunch of test cases in LogicalPlanOptimizer; more specifically, versions of all mv_expand tests that applied but with lookup join instead - barring those that would fail due to unbounded sorts (pointed out by @astefan here).

alex-spies · 2025-01-28T18:13:23Z

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

+            ROW language_code = 1
+            | MV_EXPAND language_code
+            | LIMIT 1
+            | SORT language_code
+            | LIMIT 3


This case used to not correctly take into account the LIMIT 1 at the beginning of the plan because it was absorbed into the MV_EXPAND. Now that the LIMIT 1 is duplicated rather than pushed down + absorbed, the downstream LIMIT 3 realizes that its actually a LIMIT 1 due to the upstream LIMIT 1.

alex-spies · 2025-01-28T18:36:06Z

Thanks a lot for the reviews everyone!

elasticsearchmachine · 2025-01-29T09:59:13Z

💚 Backport successful

Status	Branch	Result
✅	8.x

For queries like ... | LOOKUP JOIN lookup_index ON key | LIMIT 10 the limit cannot be simply pushed past the join - but it can be duplicated past the join. In such cases, leave an explicit Limit plan node downstream from the Join (in addition to pushing down the limit), but mark it in a way that prevents being duplicated multiple times (which would cause infinite loops). Align the logic for MV_EXPAND, which used to, instead, internalize a limit into the MvExpand node.

* ESQL: Fix LOOKUP JOIN with limit (#120411) For queries like ... | LOOKUP JOIN lookup_index ON key | LIMIT 10 the limit cannot be simply pushed past the join - but it can be duplicated past the join. In such cases, leave an explicit Limit plan node downstream from the Join (in addition to pushing down the limit), but mark it in a way that prevents being duplicated multiple times (which would cause infinite loops). Align the logic for MV_EXPAND, which used to, instead, internalize a limit into the MvExpand node. * Fix compilation

alex-spies added 6 commits January 17, 2025 13:46

Move test so all _non_unique_key tests are bundled

6d1b913

Add failing test

6e1ff52

Make progress, add another failing test

bacc841

Fix the test, expand constructor, make compile

dcfedd1

Fix tests

c61dfa4

Fix more tests

e4c1d58

elasticsearchmachine added the v9.0.0 label Jan 17, 2025

alex-spies added 6 commits January 21, 2025 12:22

Merge remote-tracking branch 'upstream/main' into fix-lookup-with-limit

5c05982

Fix serialization tests

44d5c16

Align MV_EXPAND optimization

a47a580

Remove MvExpand.limit

9c67e41

Update tests

0407b40

Avoid serializing the push down marker boolean

4afb559

alex-spies added auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.18.0 >non-issue labels Jan 21, 2025

alex-spies commented Jan 21, 2025

View reviewed changes

alex-spies marked this pull request as ready for review January 21, 2025 16:40

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 21, 2025

alex-spies requested review from costin, luigidellaquila and bpintea January 21, 2025 16:41

Merge branch 'main' into fix-lookup-with-limit

96d4285

alex-spies commented Jan 21, 2025

View reviewed changes

x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec Outdated Show resolved Hide resolved

alex-spies added 2 commits January 22, 2025 09:50

Fix unrelated tests

19d8e2a

Slightly stronger assertions

4d2bd6f

costin approved these changes Jan 24, 2025

View reviewed changes

luigidellaquila approved these changes Jan 27, 2025

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Limit.java Outdated Show resolved Hide resolved

alex-spies added 9 commits January 27, 2025 14:08

Merge remote-tracking branch 'upstream/main' into fix-lookup-with-limit

4b613c3

Address Costin's feecback

7cfebda

Simplify Limit.equals

bae6947

Add capability

90b9baf

Minor comment fixes etc.

46764e4

Improve comments and asserts in tests

13e84a6

Use a helper function for all the similar as(...) calls for Limit.class.

Skip new spec tests in CsvTests

356e98c

Fix limit serialization tests

9566b80

Streamline LocalLogicalPlanOptimizerTests changes

7705e48

alex-spies mentioned this pull request Jan 27, 2025

ESQL: LOOKUP JOIN should have SQL LEFT JOIN semantics #118781

Open

alex-spies added 2 commits January 27, 2025 17:38

More LOOKUP JOIN optimizer tests

842034a

More tests

1b16513

bpintea approved these changes Jan 28, 2025

View reviewed changes

...main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimits.java Outdated Show resolved Hide resolved

...main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimits.java Outdated Show resolved Hide resolved

alex-spies added 3 commits January 28, 2025 18:25

Merge remote-tracking branch 'upstream/main' into fix-lookup-with-limit

9717067

Copy all relevant MV_EXPAND tests for LOOKUP JOIN

a736d58

Add edge case tests for descendant limits

5ac310e

alex-spies commented Jan 28, 2025

View reviewed changes

Refactor based on Bogdan's feedback

44249dd

alex-spies added 2 commits January 28, 2025 19:36

Merge branch 'main' into fix-lookup-with-limit

75a7b19

Checkstyle

ac32bfd

alex-spies merged commit 843f1b8 into elastic:main Jan 29, 2025
16 checks passed

alex-spies deleted the fix-lookup-with-limit branch January 29, 2025 09:57

alex-spies mentioned this pull request Jan 29, 2025

[8.x] ESQL: Fix LOOKUP JOIN with limit (#120411) #121144

Merged

luigidellaquila mentioned this pull request Jan 29, 2025

ES|QL: Remove redundant sorts from execution plan #121156

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Fix LOOKUP JOIN with limit #120411

ESQL: Fix LOOKUP JOIN with limit #120411

alex-spies commented Jan 17, 2025 •

edited

Loading

alex-spies left a comment

elasticsearchmachine commented Jan 21, 2025

astefan commented Jan 22, 2025

alex-spies commented Jan 22, 2025

costin left a comment

luigidellaquila left a comment

bpintea left a comment

alex-spies left a comment

alex-spies Jan 28, 2025

alex-spies commented Jan 28, 2025

elasticsearchmachine commented Jan 29, 2025

ESQL: Fix LOOKUP JOIN with limit #120411

ESQL: Fix LOOKUP JOIN with limit #120411

Conversation

alex-spies commented Jan 17, 2025 • edited Loading

alex-spies left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jan 21, 2025

astefan commented Jan 22, 2025

alex-spies commented Jan 22, 2025

costin left a comment

Choose a reason for hiding this comment

luigidellaquila left a comment

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

alex-spies left a comment

Choose a reason for hiding this comment

alex-spies Jan 28, 2025

Choose a reason for hiding this comment

alex-spies commented Jan 28, 2025

elasticsearchmachine commented Jan 29, 2025

💚 Backport successful

alex-spies commented Jan 17, 2025 •

edited

Loading